Repository navigation
Conversation
bakpaul
added
pr: fix
|
I suppose you should adapt the propagation because of |
|
Yes it was a quick fix so Eulalie could try it, I'll do it properly. Thx |
bakpaul
added
pr: status wip
a834bcb to
2836ed3
Compare
bakpaul
added
pr: status to review
hugtalbot
added
pr: status ready
d93c272 to
5fe0263
Compare
fredroy
left a comment
fredroy
left a comment
There was a problem hiding this comment.
Claude detected some problems, one is especially concerning, as it was copying twice the velocity (copy-paste I suppose)
He (it?) also says that AccelerationBasedImplicitIntegrationScheme having the same problem.
| simulation::mechanicalvisitor::MechanicalVOpVisitor copyVelInFreeVel(params, m_vResult, core::vec_id::write_access::velocity); | ||
| copyVelInFreeVel.setMapped(true); | ||
| copyPoseInFreePose.setMapped(true); | ||
| this->getContext()->executeVisitor(©PoseInFreePose); |
There was a problem hiding this comment.
m_vop->v_eq was filtering on the eventual tags apparently, with this->getContext()->executeVisitor the tag filtering is lost. But I dont know it we still really support the tagging system.
There was a problem hiding this comment.
The tagging system might be important for some diffusion scenes. Tje v_eq was added by me in my IntegrationScheme PR so I didn't make the use of v_eq because of the tag filtering. In other words, if we don't see any changes in the tests, there is no willingness to keep filtering here.
fredroy
added
pr: status to review
|
That is only half true. I see the copy/paste error, my mistake sorry. Indeed the acceleration-based one suffer the same problem, but this is only an issue when used in a FreeMotionAnimationLoop which is never the case. I'll add it but it will be a bit useless in this case. |
Co-authored-by: Frederick Roy <fredroy@users.noreply.github.com>
25c9188 to
65911d2
Compare
hugtalbot
added
pr: status ready
…6346) * Copy local state instead of going through the mappings * Apply batched suggestions from code review Co-authored-by: Frederick Roy <fredroy@users.noreply.github.com> * Add state copy for AccelerationBased too --------- Co-authored-by: Frederick Roy <fredroy@users.noreply.github.com>
The philosophy of these new class where to use the same VecID as the current state throughout the integration to prepare the work for the Newton. This mean that the current state (position and velocity) is copied into the working state.
The issue is that after this copy, no propagation was performed. Meaning that for lagrangian-based simulation, the free states where updated with the current state but the mapped states kept the old free state. Leading to the forcefield under those mapping computing the forces values at the freestate of the previous timestep.
This PR fixes this behavior.
Some regression might fail as they might have been wrongly interpreted as non-converged CG because of very little behavior differences.
EDIT:
I have pushed another solution that behave more like the old implementation. This for me is broken mechanically speaking, but first let's bring back the old behavior with stable scenes, then we can rediscuss the ways.
[with-all-tests]
By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).
Reviewers will merge this pull-request only if